gh-145214: Narrow _GUARD_TOS_ANY_{SET,DICT} by using probable type#145215
gh-145214: Narrow _GUARD_TOS_ANY_{SET,DICT} by using probable type#145215corona10 wants to merge 4 commits intopython:mainfrom
Conversation
corona10
requested review from
Fidget-Spinner,
markshannon,
savannahostrowski and
tomasr8
as code owners
Python/optimizer_bytecodes.c
Outdated
| PyTypeObject *tp = sym_get_type(tos); | ||
| if (tp == &PySet_Type || tp == &PyFrozenSet_Type) { | ||
| ADD_OP(_NOP, 0, 0); | ||
| sym_set_type(tos, tp); |
There was a problem hiding this comment.
Why set the type when it is already known?
There was a problem hiding this comment.
Okay, now I get it means that setting the type at this moment is meaningless.
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
What we could do here is narrow the guard, if the probable type is either a set or frozen set.
So, leaving the existing check alone, add the following:
tp = sym_get_probable_type(tos);
if (tp == &PySet_Type) {
ADD_OP(_GUARD_TOS_SET, 0, 0);
}
Likewise for frozen sets, and we could also do the same for dict guards.
|
I just added a new commit:
I believe this should work, but I’m not entirely sure that I implemented _Py_uop_sym_get_probable_type correctly. |
corona10
changed the title
corona10
changed the title
Python/optimizer_bytecodes.c
Outdated
| if (sym_matches_type(tos, &PySet_Type)) { | ||
| ADD_OP(_NOP, 0, 0); | ||
| } | ||
| sym_set_type(tos, &PySet_Type); |
There was a problem hiding this comment.
We also don't need sym_set_type here? Because we checked the type in sym_matches_type.
There was a problem hiding this comment.
I’m not sure why we mark the types of tuples and other objects this way.
Maybe we need to better understand the original intention behind it.
cpython/Python/optimizer_bytecodes.c
Lines 1323 to 1349 in e234662
There was a problem hiding this comment.
We need to set it here. If you want to make it clearer, you can put an else branch, but I think it isn't needed.
There was a problem hiding this comment.
Please use an else branch, it avoids doing needless work
Co-authored-by: Ken Jin <kenjin@python.org>
|
|
|
@markshannon Gentle ping? |
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
Can you replace this pattern:
if (sym_matches_type(sym, T)) {
...
}
sym_set_type(sym, T);with this:
if (sym_matches_type(sym, T)) {
...
}
else {
sym_set_type(sym, T);
}to avoid doing pointless work.
Python/optimizer_bytecodes.c
Outdated
| if (sym_matches_type(tos, &PyDict_Type)) { | ||
| ADD_OP(_NOP, 0, 0); | ||
| } | ||
| sym_set_type(tos, &PyDict_Type); |
There was a problem hiding this comment.
| sym_set_type(tos, &PyDict_Type); | |
| else { | |
| sym_set_type(tos, &PyDict_Type); | |
| } |
Avoid the overhead of setting the type on a value we already know the type of
Python/optimizer_bytecodes.c
Outdated
| if (sym_matches_type(tos, &PySet_Type)) { | ||
| ADD_OP(_NOP, 0, 0); | ||
| } | ||
| sym_set_type(tos, &PySet_Type); |
There was a problem hiding this comment.
Please use an else branch, it avoids doing needless work
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
4 participants
Uh oh!
There was an error while loading. Please reload this page.